Collect IP and MAC address properly #161

miha-plesko · 2017-12-19T17:38:01Z

With this commit we fix a bug that prevented IP address and MAC from being collected properly. NetworkManager's refresh parser has been updated to be more robust and effective and some complex VCR unit tests were added where we ensure that things will work on a more advanced networking setup as well. Performance optimization was achieved like this:

BEFORE: first all vdcs were fetched, then all vapps per vdc, then all vms per vapp. Then finally the network data was fetched from the VM, where only the most basic data was present (e.g. MAC was missing).

NOW: we load VMs from VMDB (inventoried by cloud provider) and then invoke a single API call per VM to fetch networking data for it.

Unfortunately the Fog support for vCD is far from perfect therefore the implementation might seem somewhat hacky, but as far as I know there is no better way...

BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1520372

@miq-bot assign @agrare
@miq-bot add_label bug, enhancement, gaprindashvili/yes, fine/yes

And some screenshots:

gberginc

@miha-plesko this is just a preliminary review. I'll need to dig into it more deeply to understand the changes.

Fix the VCR issue in Travis as well.

gberginc · 2017-12-19T18:11:28Z

app/models/manageiq/providers/vmware/network_manager/refresh_parser.rb

@@ -11,14 +11,14 @@ def initialize(ems, options = nil)
      @data_index = {}
      @inv        = Hash.new { |h, k| h[k] = [] }
      @org        = @connection.organizations.first
+      @network_name_mapping = {}


Align on =

gberginc · 2017-12-19T18:12:15Z

app/models/manageiq/providers/vmware/network_manager/refresh_parser.rb

    end

    def ems_inv_to_hashes
-      log_header = "MIQ(#{self.class.name}.#{__method__}) Collecting data for EMS name: [#{@ems.name}] id: [#{@ems.id}]"


Why are you removing this, but not the next line?

Ok, I see now that log_header is now a function that you use in various places. Please see my comment there.

gberginc · 2017-12-19T18:13:40Z

app/models/manageiq/providers/vmware/network_manager/refresh_parser.rb

-    def get_networks
-      # fetch org VDC networks
-      @inv[:networks] = @org.networks
+    def log_header


move to the bottom, I guess, because it's a util function that should not interfere with the logic

gberginc · 2017-12-19T18:17:00Z

app/models/manageiq/providers/vmware/network_manager/refresh_parser.rb

-      # fetch org VDC networks
-      @inv[:networks] = @org.networks
+    def log_header
+      "MIQ(#{self.class.name}.#{__method__}) Collecting data for EMS name: [#{@ems.name}] id: [#{@ems.id}]"


log_header function must not include the part for Collecting data for ... -- this is only useful in ems_inv_to_hashes where you are showing the high level description. All other log messages should only contain information about the location of the message.

BTW, I haven't tested, but will __method__ return the name of the actual function or simply log_header function?

It'll return "log_header", if you want to do this you'll need to use caller_location like we do in vmdb_logger

Done. Thanks for noticing, I've added the debug logging ad hoc just prior opening the PR hence such silly mistakes. I should have examined the result better...

miha-plesko

Thanks for the review, I've applied the comments and repushed. Also, I've added some non-VCR unit tests for NetworkParser, please see.

miha-plesko · 2017-12-20T07:18:32Z

app/models/manageiq/providers/vmware/network_manager/refresh_parser.rb

@@ -11,14 +11,14 @@ def initialize(ems, options = nil)
      @data_index = {}
      @inv        = Hash.new { |h, k| h[k] = [] }
      @org        = @connection.organizations.first
+      @network_name_mapping = {}


miha-plesko · 2017-12-20T07:19:49Z

app/models/manageiq/providers/vmware/network_manager/refresh_parser.rb

-    def get_networks
-      # fetch org VDC networks
-      @inv[:networks] = @org.networks
+    def log_header


miha-plesko · 2017-12-20T07:28:29Z

app/models/manageiq/providers/vmware/network_manager/refresh_parser.rb

-      # fetch org VDC networks
-      @inv[:networks] = @org.networks
+    def log_header
+      "MIQ(#{self.class.name}.#{__method__}) Collecting data for EMS name: [#{@ems.name}] id: [#{@ems.id}]"


Done. Thanks for noticing, I've added the debug logging ad hoc just prior opening the PR hence such silly mistakes. I should have examined the result better...

miha-plesko · 2017-12-20T15:19:09Z

Another repush, I've added a bunch of additional non-VCR unit tests. @agrare do you think we should ping @Ladas as well since he's the "master of inventorying"?

Looking forward to be hearing your comments.

miha-plesko · 2018-01-03T15:06:05Z

@agrare all best in 2018! When you can, I kindly ask to push this PR forward so that we close the BZ at some point.

agrare · 2018-01-03T16:14:20Z

Of course @miha-plesko sorry for the delay, I'll get to it soon

miha-plesko · 2018-01-08T20:05:30Z

@agrare I realize that this specific PR seems like a hell to review 😄 But it's actually just a single file (refresh_parser.rb) with tons of VCR tests. Knowing that the current upstream refresh parser is missing a lot of data - because vmware REST API is quite a mess and not all data was scrubbed yet - you can actually consider the whole file as newly added, instead struggling with the diff.

Anyway, I'm looking forward to your review. Please let me know if I can help you in any way. I think I could even help you get access to a running vCD installation if you need?

miha-plesko · 2018-01-12T08:41:06Z

@Ladas would you care to skim over this PR? It's about network inventoring.

Ladas

Code looks good and there are nice specs, so it looks good to me. 👍

Is there some plan for Graph Refresh? It would cleanup the parser code a bit.

miha-plesko · 2018-01-12T09:31:43Z

Yes, I was thinking about graph refresh quite a lot when reimplementing, will definetly do it at some point because I don't want our provider to use ancient approaches 😄

chargio · 2018-01-18T10:38:09Z

@Ladas @agrare Can we merge this?
It looks like everyhthing is in place to merge this

agrare · 2018-01-18T13:47:48Z

app/models/manageiq/providers/vmware/network_manager/refresh_parser.rb

-      @inv[:networks] = @org.networks
+    def get_vdc_networks
+      @inv[:vdc_networks] = @org.networks
+      @inv[:vdc_networks_idx] = Hash[@inv[:vdc_networks].map { |net| [net.id, net] }]


You can just use #index_by here, @inv[:vdc_networks_idx] = @inv[:vdc_networks].index_by(&:id)

Didn't know about this one, thanks.

agrare · 2018-01-18T13:49:46Z

app/models/manageiq/providers/vmware/network_manager/refresh_parser.rb

-      # fetch org VDC networks
-      @inv[:networks] = @org.networks
+    def get_vdc_networks
+      @inv[:vdc_networks] = @org.networks


Probably a good idea to do @org.networks || [] in case this returns nil

Good point, done.

agrare · 2018-01-18T13:52:23Z

app/models/manageiq/providers/vmware/network_manager/refresh_parser.rb

+          if (vdc_net = corresponding_vdc_network(net_conf, @inv[:vdc_networks_idx]))
+            $vcloud_log.debug("#{log_header} skipping VDC network duplicate")
+            memorize_network_name_mapping(stack.ems_ref, vdc_net.name, vdc_net.id)
+            next


Instead of skipping the loop can you just move this into the else?

👍 shouldn't need the next now

agrare · 2018-01-18T13:54:25Z

app/models/manageiq/providers/vmware/network_manager/refresh_parser.rb

+    def get_network_ports
+      @inv[:nics] = []
+      @ems.vms.each do |vm|
+        fetch_nic_configurations_for_vm(vm.ems_ref).each do |nic|


Is it possible to get all NICs up front instead of one-off API calls per-VM? That way it won't slow down dramatically as the number of VMs increases (same comment for fetch_network_configurations_for_vapp)

If we can avoid an N+1 call we should definetely go for it.

VMware network refresh parser is so complicated because VMware API is surprisingly modest when it comes to networking. In other words, I wasn't able to find a better way to obtain networking information other than looping through VMs and querying for their networking configuration. API documentation that I was looking at is here: https://www.vmware.com/support/vcd/doc/rest-api-doc-1.5-html.

I'd feel much better if there were API calls like "list all networks", "list all subnets", "list all NICs", "list all Routers", "list all floating IPs" ... but they just aren't there. I'm not sure how anyone was ever able to use the API to reconstruct networking information for vCD to be honest 😄

Is it possible that VMware API is so modest with information, or did I miss something big?

Okay that is unfortunate, is this data that is usually returned with the VM but since we have to split up the parser by manager (cloud vs network) we have to get it again?
Asking because we're planning on unifying the managers into a single provider and single refresh so this issue might go away.

Yes, networking data is generally fetched together with VM data. But on this PR we paid extra attention to only fetch networking data for each VM (without other stuff about VM that we assume is already fetched by cloud provider), that's why parsing is a bit complicated (reading from XML directly).

Are you referring to graph refresh? We plan on implementing it very soon (probably as soon as this week).

Not really related to graph refresh, although that sounds great. What we want is to unify all of the managers so instead of having 2 or 3 different workers and different refreshes this could all be done in one so we wouldn't have to fetch the data again just because this is in a different manager

agrare · 2018-01-18T13:56:51Z

app/models/manageiq/providers/vmware/network_manager/refresh_parser.rb

-      uid = port_id(net_if)
-      vm_uid = net_if[:vm].id
+    def parse_network_router(router)
+      parent_id = router[:parent_net].id


If you're going to align the = below lets do the same here 😄

Woooops 😄

agrare · 2018-01-18T13:58:05Z

app/models/manageiq/providers/vmware/network_manager/refresh_parser.rb

+      }
+
+      # assign myself to the vapp network
+      @data_index.fetch_path(:cloud_subnets, "subnet-#{router[:network_id]}")[:network_router] = new_result


Think you want to use #store_path here

Nice one, thanks!

agrare · 2018-01-18T14:02:45Z

app/models/manageiq/providers/vmware/network_manager/refresh_parser.rb

+    end
+
+    def to_list(list_or_hash)
+      list_or_hash.kind_of?(Array) ? list_or_hash : [list_or_hash].compact


Should be able to just use Array(abcd) or abcd.to_a everywhere instead of this method

Unfortunately it ain't that simple because of this one example where Array() behaves really strange :

Array({ :key => 'val'}) => [[:key, "val"]] # we expect [{ :key => 'val'}]

🙄 you're right that is weird, looks like Array.wrap does the right thing though

You're right about Array.wrap, it works like a charm. Thanks, I've repushed.

agrare · 2018-01-18T14:03:37Z

@miha-plesko couple of things overall looks really good though

miha-plesko

Thanks @agrare for a great review. I'm looking forward to be hearing your opinion about VMware API modesty - can I be wrong about it?

miha-plesko · 2018-01-23T12:31:19Z

app/models/manageiq/providers/vmware/network_manager/refresh_parser.rb

-      # fetch org VDC networks
-      @inv[:networks] = @org.networks
+    def get_vdc_networks
+      @inv[:vdc_networks] = @org.networks


Good point, done.

miha-plesko · 2018-01-23T12:33:19Z

app/models/manageiq/providers/vmware/network_manager/refresh_parser.rb

-      @inv[:networks] = @org.networks
+    def get_vdc_networks
+      @inv[:vdc_networks] = @org.networks
+      @inv[:vdc_networks_idx] = Hash[@inv[:vdc_networks].map { |net| [net.id, net] }]


Didn't know about this one, thanks.

miha-plesko · 2018-01-23T12:41:04Z

app/models/manageiq/providers/vmware/network_manager/refresh_parser.rb

+          if (vdc_net = corresponding_vdc_network(net_conf, @inv[:vdc_networks_idx]))
+            $vcloud_log.debug("#{log_header} skipping VDC network duplicate")
+            memorize_network_name_mapping(stack.ems_ref, vdc_net.name, vdc_net.id)
+            next


miha-plesko · 2018-01-23T13:12:36Z

app/models/manageiq/providers/vmware/network_manager/refresh_parser.rb

+    def get_network_ports
+      @inv[:nics] = []
+      @ems.vms.each do |vm|
+        fetch_nic_configurations_for_vm(vm.ems_ref).each do |nic|


VMware network refresh parser is so complicated because VMware API is surprisingly modest when it comes to networking. In other words, I wasn't able to find a better way to obtain networking information other than looping through VMs and querying for their networking configuration. API documentation that I was looking at is here: https://www.vmware.com/support/vcd/doc/rest-api-doc-1.5-html.

I'd feel much better if there were API calls like "list all networks", "list all subnets", "list all NICs", "list all Routers", "list all floating IPs" ... but they just aren't there. I'm not sure how anyone was ever able to use the API to reconstruct networking information for vCD to be honest 😄

Is it possible that VMware API is so modest with information, or did I miss something big?

miha-plesko · 2018-01-23T13:13:12Z

app/models/manageiq/providers/vmware/network_manager/refresh_parser.rb

-      uid = port_id(net_if)
-      vm_uid = net_if[:vm].id
+    def parse_network_router(router)
+      parent_id = router[:parent_net].id


Woooops 😄

miha-plesko · 2018-01-23T13:18:13Z

app/models/manageiq/providers/vmware/network_manager/refresh_parser.rb

+      }
+
+      # assign myself to the vapp network
+      @data_index.fetch_path(:cloud_subnets, "subnet-#{router[:network_id]}")[:network_router] = new_result


Nice one, thanks!

miha-plesko · 2018-01-23T13:28:08Z

app/models/manageiq/providers/vmware/network_manager/refresh_parser.rb

+    end
+
+    def to_list(list_or_hash)
+      list_or_hash.kind_of?(Array) ? list_or_hash : [list_or_hash].compact


Unfortunately it ain't that simple because of this one example where Array() behaves really strange :

Array({ :key => 'val'}) => [[:key, "val"]] # we expect [{ :key => 'val'}]

With this commit we fix a bug that prevented IP address and MAC from being collected properly. NetworkManager's refresh parser has been updated to be more robust and effective and some complex VCR unit tests were added where we ensure that things will work on a more advanced networking setup as well. Performance optimization was achieved like this: BEFORE: first all vdcs were fetched, then all vapps per vdc, then all vms per vapp. Then finally the network data was fetched from the VM, where only the most basic data was present (e.g. MAC was missing). NOW: we load VMs from VMDB (inventoried by cloud provider) and then invoke a single API call per VM to fetch networking data for it. Signed-off-by: Miha Pleško <miha.plesko@xlab.si>

agrare

👍 LGTM, thanks @miha-plesko !

miq-bot · 2018-01-23T14:11:51Z

Some comments on commit miha-plesko@e5d4dce

spec/models/manageiq/providers/vmware/network_manager/refresh_parser_spec.rb

⚠️ - 4 - Detected allow_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.

miq-bot · 2018-01-23T14:12:00Z

Checked commit miha-plesko@e5d4dce with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
7 files checked, 4 offenses detected

**

💣 💥 🔥 🚒 - Linter/Yaml - missing config files

app/models/manageiq/providers/vmware/network_manager/refresh_parser.rb

❗ - Line 31, Col 9 - Naming/AccessorMethodName - Do not prefix reader method names with get_.
❗ - Line 41, Col 9 - Naming/AccessorMethodName - Do not prefix reader method names with get_.
❗ - Line 78, Col 9 - Naming/AccessorMethodName - Do not prefix reader method names with get_.

Collect IP and MAC address properly

Collect IP and MAC address properly (cherry picked from commit 04b4f8d) https://bugzilla.redhat.com/show_bug.cgi?id=1552673

simaishi · 2018-03-07T14:49:33Z

Gaprindashvili backport details:

$ git log -1
commit d99b0032896325f15bf62e733c9c2c1198b2be24
Author: Adam Grare <agrare@redhat.com>
Date:   Tue Jan 23 09:22:20 2018 -0500

    Merge pull request #161 from miha-plesko/ips-fix-rewrite
    
    Collect IP and MAC address properly
    (cherry picked from commit 04b4f8d7d98f2cc861d6898b7dfb190e509fee96)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1552673

Collect IP and MAC address properly (cherry picked from commit 04b4f8d) https://bugzilla.redhat.com/show_bug.cgi?id=1552675

miha-plesko · 2018-04-11T06:20:47Z

Thanks for noticing @simaishi . We don't need to backport to fine branch anymore, because we're now targeting GA onwards only. Removing fine/yes label.

@miq-bot remove_label fine/yes
@miq-bot add_label fine/no

This reverts commit 7b081db. https://bugzilla.redhat.com/show_bug.cgi?id=1552675

simaishi · 2018-04-11T17:06:28Z

Looks I accidentally had the backport pushed to Fine branch a few days ago... reverted now.

…g_of_import_error Fix message when import fails

miq-bot assigned agrare Dec 19, 2017

miq-bot added bug enhancement fine/yes labels Dec 19, 2017

gberginc suggested changes Dec 19, 2017

View reviewed changes

miha-plesko force-pushed the ips-fix-rewrite branch from 6cc0c08 to ea6a558 Compare December 20, 2017 11:23

miha-plesko commented Dec 20, 2017

View reviewed changes

miha-plesko force-pushed the ips-fix-rewrite branch 3 times, most recently from 59a9c54 to 361926a Compare December 20, 2017 14:45

Ladas approved these changes Jan 12, 2018

View reviewed changes

agrare reviewed Jan 18, 2018

View reviewed changes

miha-plesko force-pushed the ips-fix-rewrite branch from 361926a to a703ff3 Compare January 23, 2018 13:32

miha-plesko commented Jan 23, 2018

View reviewed changes

miha-plesko force-pushed the ips-fix-rewrite branch from a703ff3 to 19012e8 Compare January 23, 2018 13:47

miha-plesko force-pushed the ips-fix-rewrite branch from 19012e8 to e5d4dce Compare January 23, 2018 14:03

agrare approved these changes Jan 23, 2018

View reviewed changes

agrare merged commit e5d4dce into ManageIQ:master Jan 23, 2018

agrare added a commit that referenced this pull request Jan 23, 2018

Merge pull request #161 from miha-plesko/ips-fix-rewrite

04b4f8d

Collect IP and MAC address properly

agrare added this to the Sprint 78 Ending Jan 29, 2018 milestone Jan 23, 2018

simaishi pushed a commit that referenced this pull request Mar 7, 2018

Merge pull request #161 from miha-plesko/ips-fix-rewrite

d99b003

Collect IP and MAC address properly (cherry picked from commit 04b4f8d) https://bugzilla.redhat.com/show_bug.cgi?id=1552673

simaishi added gaprindashvili/backported and removed gaprindashvili/yes labels Mar 7, 2018

miq-bot added the gaprindashvili/yes label Mar 7, 2018

simaishi removed the gaprindashvili/yes label Mar 7, 2018

simaishi mentioned this pull request Apr 10, 2018

Skip "none" vApp network when inventoring #198

Merged

simaishi pushed a commit that referenced this pull request Apr 10, 2018

Merge pull request #161 from miha-plesko/ips-fix-rewrite

7b081db

Collect IP and MAC address properly (cherry picked from commit 04b4f8d) https://bugzilla.redhat.com/show_bug.cgi?id=1552675

miq-bot added fine/no and removed fine/yes labels Apr 11, 2018

simaishi added a commit that referenced this pull request Apr 11, 2018

Revert "Merge pull request #161 from miha-plesko/ips-fix-rewrite"

f89f47c

This reverts commit 7b081db. https://bugzilla.redhat.com/show_bug.cgi?id=1552675

miha-plesko deleted the ips-fix-rewrite branch January 7, 2019 08:25

agrare pushed a commit to agrare/manageiq-providers-vmware that referenced this pull request Apr 15, 2019

Merge pull request ManageIQ#161 from jelkosz/vm_import_change_phrasin…

f590f04

…g_of_import_error Fix message when import fails

Collect IP and MAC address properly #161

Collect IP and MAC address properly #161

Conversation

miha-plesko commented Dec 19, 2017

gberginc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miha-plesko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miha-plesko commented Dec 20, 2017

miha-plesko commented Jan 3, 2018

agrare commented Jan 3, 2018

miha-plesko commented Jan 8, 2018

miha-plesko commented Jan 12, 2018

Ladas left a comment

Choose a reason for hiding this comment

miha-plesko commented Jan 12, 2018

chargio commented Jan 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agrare commented Jan 18, 2018

miha-plesko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agrare left a comment

Choose a reason for hiding this comment

miq-bot commented Jan 23, 2018

miq-bot commented Jan 23, 2018

simaishi commented Mar 7, 2018

miha-plesko commented Apr 11, 2018

simaishi commented Apr 11, 2018